docs(spec-sync): scope the wiring prompt to request fields, record the aliases - #122
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The gate can fail open, misses precedence conflicts, and records behavior not yet implemented.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CI validation to keep V2 convenience aliases aligned with OpenAPI request fields and cross-SDK precedence rules.
Changes:
- Adds a shared alias manifest and validation script.
- Integrates validation into lint and spec-sync workflows.
- Documents request-field synchronization requirements.
File summaries
| File | Review |
|---|---|
specs/v2-aliases.json |
Declares parse password precedence that does not match current behavior; land/rebase #121 with conflict tests first. |
scripts/spec-sync/check-v2-aliases.sh |
Must resolve referenced request bodies and validate precedence when aliases target top-level fields. |
scripts/lint |
Runs the new validation gate. |
CONTRIBUTING.md |
Documents alias synchronization rules. |
.github/workflows/spec-sync.yml |
Adds field validation and repair guidance. |
Review details
Suppressed comments (1)
scripts/spec-sync/check-v2-aliases.sh:271
- The Node fallback has the same fail-open behavior for an OpenAPI operation whose
requestBodyis a$ref: it marks the path as body-bearing but emits no fields, omitting the path from the operations that are checked. Dereference the request body before iterating its content.
if (!isObj(op) || !isObj(op.requestBody)) continue;
hasBody = true;
for (const media of Object.values(op.requestBody.content || {})) {
if (isObj(media)) walk(isObj(media.schema) ? media.schema : {}, "", 1, found);
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tian-lan-landing
added a commit
to landing-ai/ade-python
that referenced
this pull request
Sep 9, 2026
From Copilot review on #161 / landing-ai/ade-typescript#122. - An operation is now recorded from an "O" row, emitted whenever it has a requestBody at all, instead of being inferred from the fields found in it. A body with no top-level properties used to disappear from the operation list entirely, so a NEW /v2 route with an empty or referenced body passed with exit 0 — its missing OPERATIONS mapping and any params it declared went unchecked. - `requestBody` is dereferenced before reading `content`, so a body behind a `$ref` (valid OpenAPI; this generator inlines, but that is not a guarantee) reads the same as an inline one. A `$ref` that does not resolve is reported against the spec — it used to surface as a misleading complaint about the manifest. - A manifest target naming a TOP-LEVEL spec field no longer skips the precedence check. The target is itself the thing a caller sets directly, so it decides whether a conflict is possible: "sole" is now rejected when the SDK declares it. Both JSON readers keep producing identical dumps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing
added a commit
that referenced
this pull request
Sep 9, 2026
From Copilot review on #122 / landing-ai/ade-python#161. - An operation is now recorded from an "O" row, emitted whenever it has a requestBody at all, instead of being inferred from the fields found in it. A body with no top-level properties used to disappear from the operation list entirely, so a NEW /v2 route with an empty or referenced body passed with exit 0 — its missing OPERATIONS mapping and any params it declared went unchecked. - `requestBody` is dereferenced before reading `content`, so a body behind a `$ref` (valid OpenAPI; this generator inlines, but that is not a guarantee) reads the same as an inline one. A `$ref` that does not resolve is reported against the spec — it used to surface as a misleading complaint about the manifest. - A manifest target naming a TOP-LEVEL spec field no longer skips the precedence check. The target is itself the thing a caller sets directly, so it decides whether a conflict is possible: "sole" is now rejected when the SDK declares it. Both JSON readers keep producing identical dumps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved request bodies currently produce misleading undeclared-field errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
…e aliases check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#121, landing-ai/ade-python#160). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing
force-pushed
the
ci/v2-alias-parity-gate
branch
from
September 9, 2026 09:24
2119502 to
8fa57f4
Compare
tian-lan-landing
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The diff does not include the CI gate, manifest, or lint wiring advertised by the PR metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Collaborator
Author
|
Branch rewritten: this is now prompt + docs only. The earlier Copilot threads on this PR were about that removed script (and, in ade-typescript, the removed manifest), so they no longer apply to the diff. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The wiring prompt scoped
client.v2to the spec's/v2routes and said nothing about which fields back a/v2request. That is how the parse password went wrong: the spec declares it only atoptions.password, the same spec still declares a top-levelpasswordon/v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere (#121 / landing-ai/ade-python#160).Prompt and docs only — no code, no new script,
scripts/lintand the CI gates untouched./v1top-level field is not a/v2field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one.password→options.password,strict→options.strict), the precedence rule for each, the language-specific traps on both sides, and — plainly — that nothing in CI checks this, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand.Enforcing it mechanically was considered and rejected: a gate can check that a rule has been written down somewhere, not that the code obeys it, and that is not worth the machinery. The risk stays with review.
The documented
passwordprecedence is what #121 implements in this repo, so land #121 for the doc to be true here. Paired with landing-ai/ade-python#161.